feat(offline_first_with_rest): add request/response callbacks to the RestOfflineQueueClient #447
Conversation
…RestOfflineQueueClient
![jit-ci[bot]](https://avatars.githubusercontent.com/in/142441?s=60&v=4){kind=small}
There was a problem hiding this comment.
@devj3ns Thanks for opening this PR. A few high level notes -
- I'm still concerned about
http.StreamedResponse. From the discussion on Proposal: add the response details to the offline request queue #393:
Yes, I'm aware of that. In my case, the HTTP status code provides enough information, so this limitation shouldn't be an issue.
This may be the way your app uses this function, however, you'd be amazed at how people use Brick in ways you did not expect them to. If you expose functionality, people expect to use it. So either 1) http.StreamedResponse needs to be passed as a parsed object or 2) only the status code can be passed to the callbacks or 3) a unit test needs to prove that the response can be parsed twice.
-
This feature needs unit testing. If you're just throwing up this PR to get architecture discussion, we can come back to unit testing when the arch is finalized.
-
This requires an equivalent in the GraphQL domain. From the Proposal: add the response details to the offline request queue #393 discussion:
I tried this, but ran into an issue I currently do not know how to resolve properly: the RestOfflineRequestQueueClient relies on the http.StreamedResponse type for a response, while GraphqlOfflineQueueLink uses the Response type.
OfflineRequestQueue has a generic type of TRequest that should provides the placeholder for the Request/http.Request. A second generic type of TResponse can be provided for Response/http.StreamedResponse on `OfflineRequestQueue.
- There needs to be documentation in the README, the CHANGELOG, and the
docs/Markdown. Again, can wait until we're settled on architecture.
Overall, this approach is simple. Not using a Stream feels like an anti-pattern...but then you'd have to manage a StreamController and handle the dispose, which would require state, and Brick shouldn't be stateful. But there's a lingering feeling that the callback-stored-as-instance-field doesn't feel quite right... @mateominato do you have any ideas here?
packages/brick_offline_first_with_rest/lib/src/offline_first_with_rest_repository.dart
Outdated
Show resolved
Hide resolved
| /// A callback triggered when the response of a request has a status code | ||
| /// which is present in the [reattemptForStatusCodes] list. | ||
| void Function(http.Request request, http.StreamedResponse response)? | ||
| onReattemptableResponse; |
There was a problem hiding this comment.
Would prefer to keep the names similar
| /// A callback triggered when the response of a request has a status code | |
| /// which is present in the [reattemptForStatusCodes] list. | |
| void Function(http.Request request, http.StreamedResponse response)? | |
| onReattemptableResponse; | |
| /// A callback triggered when the response of a request has a status code | |
| /// which is present in the [reattemptForStatusCodes] list. | |
| void Function(http.Request request, http.StreamedResponse response)? | |
| onReattemptResponse; |
There was a problem hiding this comment.
Is the onReattemptResponse necessary? Your PR description notes that it can help resolve a blocked queue, but is it clear how many retries have been made on this request? Is that necessary to determine when to skip a request? If you've explicitly told Brick "reattempt in this scenario" and now you want to no longer attempt for that scenario, shouldn't you remove the status code from the reattempt list instead?
There was a problem hiding this comment.
The onReattemptResponse helps to
- know that the client is online (because there was a response from the server)
- check if the status code of the returned response code indicates that the server rejected the response (e.g., HTTP 409) (in which case I prompt the user to handle the conflict)
If you've explicitly told Brick "reattempt in this scenario" and now you want to no longer attempt for that scenario, shouldn't you remove the status code from the reattempt list instead?
If I removed the status code, (e.g., HTTP 409) from the reattempt from status codes list, the client had no chance to detect and handle the rejected response and the data is lost.
There was a problem hiding this comment.
Would prefer to keep the names similar
I think naming the callback onReattemptResponse is misleading, as it sounds like a response would be reattempted. But my initial suggestion onReattemptableResponse has the same problem.
What about onReattemptableRequestFailed?
There was a problem hiding this comment.
What about onRequestFailure?
There was a problem hiding this comment.
So you would go with onRequestFailure and onRequestError? Then it's difficult to guess from the name which one receives which callback.
What about
onReattemptableRequestFailure and onRequestException or
onResponse and onRequestException?
packages/brick_offline_first_with_rest/lib/src/offline_queue/rest_offline_queue_client.dart
Outdated
Show resolved
Hide resolved
packages/brick_offline_first_with_rest/lib/src/offline_queue/rest_offline_queue_client.dart
Show resolved
Hide resolved
packages/brick_offline_first_with_rest/lib/src/offline_queue/rest_offline_queue_client.dart
Outdated
Show resolved
Hide resolved
|
Thanks for the feedback @tshedor, I'm currently on vacation and will respond to your feedback and update the PR next week. |
…hods instead of params
|
Sorry for the late response @tshedor! I had a very busy last week... I have already addressed some of your feedback and will address the rest this evening.
Yes! I will add tests once we have agreed on an implementation.
Same as with the tests. |
|
@tshedor, changed the implementation to only include the response status code instead of the
I'm in favor of creating the generic methods in the |
This PR introduces two optional callbacks to the
RestOfflineQueueClient, which closes #393.onReattemptableResponse: This callback is invoked when a request returns a response with a status code listed in the reattemptForStatusCodes. It allows for detecting cases where a request is repeatedly rejected by the remote provider (e.g., when receiving an HTTP 409 (Conflict) response). When serial processing is enabled, this helps detect a blocked queue, allowing the app implementation to handle it appropriately.onRequestError: This callback is triggered when a request throws an exception during execution (e.g., a SocketException). It helps identify the current state of the offline queue synchronization process.In my application, these two callbacks are used to monitor the status of the offline queue, determining whether the queue is actively processing, blocked by a rejected request, or paused due to the client being offline. This allows me to provide users with real-time feedback on the system's sync status, which is important for my application.
@tshedor, what do you think of this implementation?